Fix webpack loader cache key for resource queries#19723
Fix webpack loader cache key for resource queries#19723RobinMalfait merged 4 commits intotailwindlabs:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Webpack loader's cache handling was refactored to derive cache keys from resource identifiers via a new helper 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/@tailwindcss-webpack/src/index.ts (1)
43-45: Consider a collision-proof separator for the composite cache key.The
:delimiter is also present in Windows drive-letter paths (e.g.,opts.base = "C:/some/dir"), making the key format theoretically ambiguous. In practice this is unlikely to cause a collision today, but the fix is trivial.♻️ Suggested improvement
function getCacheKey(resourceId: string, opts: LoaderOptions): string { - return `${resourceId}:${opts.base ?? ''}:${JSON.stringify(opts.optimize)}` + return JSON.stringify([resourceId, opts.base ?? '', opts.optimize ?? null]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@tailwindcss-webpack/src/index.ts around lines 43 - 45, The composite cache key in getCacheKey(resourceId, opts) uses ":" which can collide with Windows drive letters; update getCacheKey to produce an unambiguous key—either by using a collision-proof separator (e.g., a null character '\u0000' or an unlikely token) between resourceId, opts.base and JSON.stringify(opts.optimize), or simply serialize the whole object (e.g., JSON.stringify({resourceId, base: opts.base ?? '', optimize: opts.optimize})) so the key is unambiguous; change the implementation in getCacheKey to use one of these approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/`@tailwindcss-webpack/src/index.ts:
- Around line 43-45: The composite cache key in getCacheKey(resourceId, opts)
uses ":" which can collide with Windows drive letters; update getCacheKey to
produce an unambiguous key—either by using a collision-proof separator (e.g., a
null character '\u0000' or an unlikely token) between resourceId, opts.base and
JSON.stringify(opts.optimize), or simply serialize the whole object (e.g.,
JSON.stringify({resourceId, base: opts.base ?? '', optimize: opts.optimize})) so
the key is unambiguous; change the implementation in getCacheKey to use one of
these approaches.
Otherwise the embedded scripts might get confused.
RobinMalfait
left a comment
There was a problem hiding this comment.
I've updated the integration test to make it pass in CI. The \n were causing a bit of issues because we were embedding it in JS to write to the file, so I think we should've used \\n but I got rid of it anyway.
Thanks!
Summary
@tailwindcss/webpackcurrently usesthis.resourcePathas the cache key, which ignores the resource query. When the same CSS file is imported multiple times with differentresourceQueryvalues, all of those imports share a singleCacheEntry. That means the utilities discovered for one entry can leak into the CSS output for another entry.This PR changes the cache key to use
this.resource(path + query) instead, while still usingthis.resourcePathfor all filesystem work.Test plan
pnpm test:integrations -- webpack/loader.test.ts@tailwindcss/webpack loader isolates cache by resource including querytest passes, verifying that two entries importing the same CSS file with different queries produce isolated outputs (dist/a.cssonly containsonly-a/--color-red-500, anddist/b.cssonly containsonly-b/--color-blue-500).